Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Respect specified ValueTypes for features during materialization #1906

Merged
merged 8 commits into from
Sep 29, 2021
Merged

Respect specified ValueTypes for features during materialization #1906

merged 8 commits into from
Sep 29, 2021

Conversation

Agent007
Copy link
Contributor

@Agent007 Agent007 commented Sep 23, 2021 •
edited
Loading

What this PR does / why we need it:
Float features are erroneously materialized to online stores as doubles. This bug could drastically affect the memory and storage usage of online stores.

Which issue(s) this PR fixes:

Fixes #1904

Does this PR introduce a user-facing change?:

NONE

@feast-ci-bot
Copy link
Collaborator

Hi @Agent007. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Agent007
Copy link
Contributor Author

/kind bug

@Agent007 Agent007 marked this pull request as ready for review September 23, 2021 22:41
@Agent007
Copy link
Contributor Author

@judahrand , you may be interested in this pull request since you've been working on cleaning up type conversions.

@Agent007 Agent007 changed the title assert float feature is still float from online store ensure float features retain float type from online store Sep 24, 2021
Agent007 and others added 3 commits September 25, 2021 16:17
@Agent007
assert float feature is still float from online store
2231ef4 
Signed-off-by: Jeff <[email protected]>
@Agent007
ensure float features retain float type from online store
eb2bcfd 
Floats were converted to doubles when materialized to
the online store.  There is a broader bug trend around
type conversions and this particular conversion utility
function looks like it could use some cleanup. This
commit is a quick fix.

Signed-off-by: Jeff <[email protected]>
@achals
make fix more general
2819dd3 
Signed-off-by: Achal Shah <[email protected]>
@achals
Copy link
Member

achals commented Sep 28, 2021

@Agent007 I've updated this PR with what I think is a more general fix. Thanks for your contribution! If it looks good after tests, i'll merge it in today.

@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2021 •
edited
Loading

Codecov Report

Merging #1906 (badcdf8) into master (0342aa4) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1906      +/-   ##
==========================================
- Coverage   83.05%   82.83%   -0.22%     
==========================================
  Files          96       96              
  Lines        7317     7360      +43     
==========================================
+ Hits         6077     6097      +20     
- Misses       1240     1263      +23
Flag Coverage Δ
integrationtests 74.60% <66.66%> (-0.21%) ⬇️
unittests 60.65% <44.44%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdk/python/feast/type_map.py 72.59% <100.00%> (-8.89%) ⬇️
...n/tests/integration/online_store/test_e2e_local.py 100.00% <100.00%> (ø)
.../integration/online_store/test_universal_online.py 100.00% <100.00%> (ø)
sdk/python/feast/repo_operations.py 42.46% <0.00%> (-1.76%) ⬇️
sdk/python/tests/unit/test_entity.py 100.00% <0.00%> (ø)
sdk/python/feast/feature_store.py 93.80% <0.00%> (+0.01%) ⬆️
sdk/python/feast/feature_view.py 86.92% <0.00%> (+0.35%) ⬆️
sdk/python/feast/on_demand_feature_view.py 89.65% <0.00%> (+0.36%) ⬆️
sdk/python/feast/entity.py 74.80% <0.00%> (+0.81%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0342aa4...badcdf8. Read the comment docs.

@achals
Use assertAlmostEquals
8d270e8 
Signed-off-by: Achal Shah <[email protected]>
@achals
format
07a1aa2 
Signed-off-by: Achal Shah <[email protected]>
@Agent007
Copy link
Contributor Author

@achals Thanks!

@achals achals changed the title ensure float features retain float type from online store Respect specified ValueTypes for features during materialization Sep 29, 2021
achals
achals approved these changes Sep 29, 2021
View reviewed changes
Copy link
Member

@achals achals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, Agent007

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit ce5a130 into feast-dev:master Sep 29, 2021
@Agent007 Agent007 deleted the prevent-float-features-to-double-conversion branch September 29, 2021 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achals achals achals approved these changes

@adchia adchia Awaiting requested review from adchia

@felixwang9817 felixwang9817 Awaiting requested review from felixwang9817

@tsotnet tsotnet Awaiting requested review from tsotnet

@woop woop Awaiting requested review from woop

Assignees

@achals achals

Labels
approved kind/bug lgtm ok-to-test release-note-none size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Float features are materialized into online stores as doubles
4 participants
@Agent007 @feast-ci-bot @achals @codecov-commenter